Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2.x composite disposable docs #6432

Merged
merged 4 commits into from
Mar 15, 2019
Merged

2.x composite disposable docs #6432

merged 4 commits into from
Mar 15, 2019

Conversation

chronvas
Copy link

On CompositeDisposable add and addAll methods, if the param is null, currently the NPE error message (produced by ObjectHelper) is "d is null" which is not very helpful.

This is a small refactor for making the message a bit more helpful.

Resolves #6430

Copy link
Member

@akarnokd akarnokd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording needs further adjustments.

for (Disposable d : resources) {
ObjectHelper.requireNonNull(d, "Disposable item is null");
public CompositeDisposable(@NonNull Disposable... disposables) {
ObjectHelper.requireNonNull(disposables, "Disposables are null");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use lowercase as it refers to the argument: disposables is null.

ObjectHelper.requireNonNull(disposables, "Disposables are null");
this.resources = new OpenHashSet<Disposable>(disposables.length + 1);
for (Disposable d : disposables) {
ObjectHelper.requireNonNull(d, "Disposable item in Disposables is null");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, lowercase: A Disposable in the disposables array is null.

public CompositeDisposable(@NonNull Iterable<? extends Disposable> resources) {
ObjectHelper.requireNonNull(resources, "resources is null");
public CompositeDisposable(@NonNull Iterable<? extends Disposable> disposables) {
ObjectHelper.requireNonNull(disposables, "Disposables are null");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disposables is null.

@@ -38,26 +38,28 @@ public CompositeDisposable() {

/**
* Creates a CompositeDisposables with the given array of initial elements.
* @param resources the array of Disposables to start with
* @param disposables the array of Disposables to start with
* @throws NullPointerException if disposables param is null, or a Disposable item in the disposables is null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if {@code disposables} or any of its array items is null

this.resources.add(d);
}
}

/**
* Creates a CompositeDisposables with the given Iterable sequence of initial elements.
* @param resources the Iterable sequence of Disposables to start with
* @param disposables the Iterable sequence of Disposables to start with
* @throws NullPointerException if disposables param is null, or a Disposable item in the disposables is null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if {@code disposables} or any of its returned items is null

* @return true if the operation was successful, false if the container has been disposed
* @throws NullPointerException if disposables param is null, or a Disposable item in the disposables is null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if {@code disposables} or any of its array items is null

public boolean addAll(@NonNull Disposable... ds) {
ObjectHelper.requireNonNull(ds, "ds is null");
public boolean addAll(@NonNull Disposable... disposables) {
ObjectHelper.requireNonNull(disposables, "Disposables are null");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disposables is null

for (Disposable d : ds) {
ObjectHelper.requireNonNull(d, "d is null");
for (Disposable d : disposables) {
ObjectHelper.requireNonNull(d, "Disposable item in Disposables is null");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Disposable in the disposables array is null

* @return true if the operation was successful
* @throws NullPointerException if disposable param is null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if {@code disposable} is null

public boolean delete(@NonNull Disposable d) {
ObjectHelper.requireNonNull(d, "Disposable item is null");
public boolean delete(@NonNull Disposable disposable) {
ObjectHelper.requireNonNull(disposable, "Disposable item is null");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disposable is null

@codecov
Copy link

codecov bot commented Mar 15, 2019

Codecov Report

Merging #6432 into 2.x will decrease coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6432      +/-   ##
============================================
- Coverage     98.34%   98.27%   -0.07%     
+ Complexity     6300     6299       -1     
============================================
  Files           675      675              
  Lines         45172    45172              
  Branches       6246     6246              
============================================
- Hits          44425    44394      -31     
- Misses          229      244      +15     
- Partials        518      534      +16
Impacted Files Coverage Δ Complexity Δ
.../io/reactivex/disposables/CompositeDisposable.java 100% <100%> (ø) 40 <21> (ø) ⬇️
...nternal/operators/parallel/ParallelReduceFull.java 91.08% <0%> (-3.97%) 2% <0%> (ø)
...tivex/internal/schedulers/TrampolineScheduler.java 96.1% <0%> (-2.6%) 6% <0%> (ø)
...va/io/reactivex/internal/queue/SpscArrayQueue.java 97.61% <0%> (-2.39%) 22% <0%> (-1%)
...ava/io/reactivex/processors/BehaviorProcessor.java 96.86% <0%> (-2.25%) 60% <0%> (ø)
.../internal/disposables/ListCompositeDisposable.java 98% <0%> (-2%) 34% <0%> (-1%)
...internal/operators/observable/ObservableCache.java 95.9% <0%> (-1.64%) 34% <0%> (-1%)
...rnal/operators/observable/ObservableSwitchMap.java 94.68% <0%> (-1.6%) 3% <0%> (ø)
...ternal/operators/completable/CompletableMerge.java 96.42% <0%> (-1.2%) 2% <0%> (ø)
.../io/reactivex/internal/schedulers/IoScheduler.java 91.48% <0%> (-1.07%) 9% <0%> (ø)
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a74adf...e909e08. Read the comment docs.

@chronvas
Copy link
Author

Thanks for your feedback, I think I addressed all of your suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants